Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add HandlerWithAllowlist which can control exposed metrics #1327

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

CatherineF-dev
Copy link

@CatherineF-dev CatherineF-dev commented Aug 12, 2023

After adding HandlerWithAllowlist, cilium or other components can exclude high cardinality metrics instead of disabling metrics collection in a running cluster.

context: cilium/cilium#27471

I notice this pr introduces a new dependency (dto "github.com/prometheus/client_model/go"), maybe need to move HandlerWithAllowlist to another file.

Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Normally this could be done on registration, so you only register what you want.

I guess you want to register all and filter afterwards in one place. This has consequences, one would check in the code that certain metric is instrumented but it's actually ignored near the handler.

Still, I think it's fine, however there is no need to add complexity to handler AND add new gatherer. We could simply add new gatherer similar to what you have here. Anyone can then wrap their registry with it.

@@ -381,6 +393,35 @@ type HandlerOpts struct {
ProcessStartTime time.Time
}

type FilteredDefaultGatherer struct {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would just add FilteredGatherer that wraps registry and pass through only matching metrics. Matchers can be done with something like https://github.com/prometheus/client_golang/blob/main/prometheus/collectors/go_collector_latest.go#L93 - regexp.

This allows us to allow list those metrics you wanted to hardcode here, outside of FilteredGatherer - on caller side (:

prometheus/promhttp/http.go Outdated Show resolved Hide resolved
Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it still needed? I noticed the CFP/feature request was closed.

Nevertheless it feels useful, just we might want to discuss an exact API. Thanks! 🤗

@@ -90,6 +93,13 @@ func HandlerFor(reg prometheus.Gatherer, opts HandlerOpts) http.Handler {
return HandlerForTransactional(prometheus.ToTransactionalGatherer(reg), opts)
}

// HandlerWithAllowlist remove metrics whose name can't match with metricNameMatcher
func HandlerWithAllowlist(metricNameMatcher *regexp.Regexp) http.Handler {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is nice, but we might want to discuss exact API. e.g. do we want only metric names or also labels? What would be flexible enough?

We think it might be useful to tackle that with OpenMetrics changes and have same API for per scrape or per handler options 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants